Fix/chunk test suite stale num pubs - #20
Open
yeziR4 wants to merge 85 commits into
Open
Conversation
* chore: fix array out-of-bounds error with index validation * Update mmr_guest.rs * Update header-chain/src/mmr_guest.rs Co-authored-by: Aaron Feickert <66188213+AaronFeickert@users.noreply.github.com> --------- Co-authored-by: Aaron Feickert <66188213+AaronFeickert@users.noreply.github.com>
* WIP: Bigint configuration * feat: bigint transforms fuzzing * test: bigint limb transforms fuzzing -add: macro for cleaning, adjusted test parameter, and idiomatic cleanup * clean: separate files and remove unncessary struct param * chore: cargo fmt * Revert "chore: cargo fmt" This reverts commit 0ea384a. * chore: cargo formatting error, remove single line enforcement * chore: move stack element verify script to fuzz library
BitVM#282) * Fix connector_b_leaf_2_script test for proper Taproot script verification * Update connector_b.rs * Update connector_b.rs * fix cargofmt
* chore: remove paste macro
I simply transform `paste! { X }` into `X`. If the diff for this commit
is large, then git is having an issue. The `paste!` macro is used
elsewhere in the codebase, so I won't remove the dependency.
* chore: clippy
cargo clippy --fix --lib -p bitvm
* chore: replace static with const
* chore: Inline
* refactor: Replace macros with functions
Functions are easier to read and tools like `rustfmt` and `clippy` have
an easier time parsing them.
* refactor: clarify endianness
* doc: winternitz
* refactor: Rename "blocks" to "digits"
This commit renames "blocks" to "digits" in function signatures and
struct members. I used the opportunity to clearly separate lengths
in units of digits from lengths in units of bits.
This commit touches a lot of code. The Parameter struct is technically
public and changed by this commit. However, I argue that no one should
manually set up his parameters. Instead, everyone should use the
high-level methods for wots-hash, wots-blake and wots256, which are not
affected by this commit.
I plan to make the Parameter struct private in a follow-up PR.
* doc: address comments
Only changes to docstrings and parameter names.
No changes to public API.
* fix: documentation
* chore: Mark potential footgun * feat: New WOTS API * test: Add WOTS test vectors The test vectors are generated directly in Rust, so we can add new vectors or modify existing vectors in the future. * refactor: Adapt bitvm crate to new WOTS API Removes the dependency on the modules wots_api and signing_winternitz. * refactor: adapt bridge to new wots api Removes the dependency on wots_api. Removing signing_winternitz turned out to be too much work, as it touches a lot of code. In particular, the connector outputs handle public keys for different message lengths in the same vector. The bridge code can be refactored in the future if needed. * refactor: Move winternitz_hash into signing_winternitz signing_winternitz is the only place that calls winternitz_hash, so why not combine the modules? BLAKE3 switched from 20-byte WOTS signatures to Wots16 a while ago, so I use the opportunity to remove the code for 20-byte WOTS. * chore: Remove wots_api.rs No more code depends on this, so it is safe to remove. * fix: Address comments * refactor: WinternitzSecret constructors Deprecate from_string constructors. Add from_bytes as a safer alternative. * fix: Address comments * fix: address comments * fix: cargo fmt
* feat: add tmul for fp2 with 4 window stack len * feat: add fp2_w4 in ss_mul to reduce script size * feat: add w4 version of lc4_keep_elements * chore: specify vec capactiy * doc: add docs to new functions
* zellic g1 fixes * test for read from stack * deleted hinted_x_from_eval_point and hinted_y_from_eval_point functions * fmt * used fq::is_zero instead of comparing bytes to zero * changed & to &&
* removed unnecessary log asserts * removed unnecessary if branch in tonelli shanks * removed unused imports
* fix fq6_hinted_mul_keep_elements * removed unused import * fmt
…put if NUM_PUBS > 1 (BitVM#369) * fixed num_pub>1 case * fixed num_pubs>1 case
* add empty stack check * fix comment
…itVM#371) * drop c left in the altstack * remove if branch in utils_fq12_square since it doesnt occur
…, and add note in tmul about a case about hint (BitVM#386)
* fix checksum length * add warning for repeated hashes * fix pushing digits * fmt * add hash length checks and comment verify_test_vector's temporarily since its not compatible with the new checksum length * fix secret key generation * remove BinarysearchVerifier bound check and add test to verify that it doesn't cause a vulnerability * fix raw_witness_to_signature digit conversion
* add validation for hint being a correctly formed T type value to tmul * check that hint y is valid in Fp254Impl::hinted_inv * add check_validity to places where hints are used * add Fq::check_validity and use it in necessary place * add hint validity checks to chunk::taps_points:ops::utils_point_add_eval * chore: rename check validity functions for cherry-picking optimized version * feat: add check_validity function to Fp254 * chore: incorperate cherry-picked version * opt: optimize copies for hint verifications with the cherry-picked verification * fmt --------- Co-authored-by: just-erray <c.e.aslan37@gmail.com>
* fix fp254impl issues * fmt * fmt * add back necessary conflicted functions
* clear part1 report * add assertion to push_u32_le * fmt * remove debug prints and improve test for limb_shr1_carry * fmt * limb_to_bits functions assert num_bits>0 * change G1Affine::roll, G2Affine::roll, G2Affine::copy parameters to in terms of Fq, and fix tests for bigint change * fmt * assert LIMB_SIZE < 31 in BigIntImpl * restore bigint/mul.rs because it is fixed in another pr * fmt * apply parameter change to double_prevent_overflow_keep_element too
* feat: add check_validity function to Fp254 * feat: add validity checks to some functions in taps_ext_miller.rs as samples * add validity check to taps_mul * feat: add is_valid function to Fp254 * fix-feat: fix validity checks for precompute_p and complete validity checks for taps_ext_miller.rs * feat: fix and add validity checks in taps_msm.rs * fix: a validity check in taps_points_ops.rs * fix-feat: correct and add validity checks * fix: correct tests for test_point_ops_and_multiply_line_evals_step_1 tests according to the changed form of the input * fix: add validity checks for G1Acc in chunk_msm * opt: remove unnecessary q4 validity check * fix: validity checks of taps_ext_miller::chunk_precompute_p_from_hash * feat: add functions to analyze max stack usage of chunks * fix: some typos and delete leftovers --------- Co-authored-by: Hakkush-07 <hakan.karakus@std.bogazici.edu.tr>
* create a version of G1Affine::hinted_check_add that the caller must use with points that are not t==q nor t==-q * optimize G1Affine::hinted_check_add and add TODO comments for later * rearrange wrong if checks in G1Affine::hinted_check_add * add validity checks to hints of new G1Affine::hinted_check_add * fix hint validity check and roll in G1Affine::hinted_check_add
Sync BitVM updates
…etwork#12) * remove length-prefix when get groth16 pubin * fix guest inputs verify script
generate_segments_using_mock_vk_and_mock_proof moved to
api_compiletime_utils in an earlier refactor; this test module's import
was never updated, so `cargo test -p bitvm chunk::` has been failing to
compile outright.
With this fix applied, the suite compiles and reveals 7 further test
failures (chunk::api::test::full_e2e_execution, test_largest_chunks,
chunk::api_runtime_utils::test::test_runtime_execution_looped,
chunk::g16_runner_core::test::{test_groth16,test_verify_pairing},
chunk::taps_msm::test::{test_tap_msm_valid_inputs,test_tap_msm_invalid_inputs_scalar_not_fr}),
all tracing to one shared hardcoded test fixture (vk_bytes/proof_bytes/
scalar) built for a 1-public-input verifying key, while NUM_PUBS has been
2 since the Nov 21 2025 merge-conflict-resolution commit 7b31dfd. That
mismatch is a separate, still-open issue (not fixed here — needs a
regenerated 2-public-input fixture) filed alongside this PR.
Not believed to be exploitable in production: the live Ziren integration
test in goat::disprove_scripts (test_verify_guest_pubin_ziren, using a
real 2-element ziren/public_inputs.bin) passes cleanly. The risk is pure
test-suite/CI coverage: this module's regression suite for the chunked
Groth16 pairing verifier has evidently not produced a passing run in
~9 months.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cargo test -p bitvm --release chunk::on this branch currently fails to compile before any test runs:generate_segments_using_mock_vk_and_mock_proofwas moved toapi_compiletime_utilsin an earlier refactor; the#[cfg(test)] mod testinchunk/api.rswas never updated to import it from its new location. This one-line fix restores the import and unblocks compilation.What this uncovers (not fixed in this PR)
With the import restored, the suite compiles and 7 tests fail:
All 7 trace to the same root cause: a hardcoded test fixture (
vk_bytes/proof_bytes/ a singlescalar) shared across five test functions was built from a verifying key with 1 public input.NUM_PUBS(bitvm/src/chunk/api.rs) has been2since the merge-conflict-resolution commit 7b31dfd (Nov 21, 2025) — roughly 9 months. Representative panics:I don't believe this is exploitable in production —
goat::disprove_scripts::test_verify_guest_pubin_ziren, which deserializes a real 2-element public-input array fromziren/public_inputs.bin, passes cleanly, so the live Ziren integration matchesNUM_PUBS = 2correctly. This also isn't the Zellic-audited MSM finding (3.19, commit 9f2a42a) — that was a Script stack-drop bug intaps_msm.rswith no test-fixture changes; this is a separate, distinct gap.The real-world impact is coverage risk: the regression suite for the chunked Groth16 pairing verifier — the code that ultimately decides whether a cheating operator gets caught on-chain — has evidently not produced a passing
cargo test -p bitvmrun in ~9 months, meaning a genuine regression introduced anywhere inchunk::during that window would not have been caught.This PR intentionally fixes only the compile break. Regenerating the shared fixture with a real 2-public-input proof (or parameterizing the affected tests to read
NUM_PUBSfrom the VK's owngamma_abc_g1.len() - 1instead of assuming a constant) needs a decision from the team on the right fixture/approach, so I've filed that separately rather than guessing at a replacement proof.Testing
Before: compile error. After this PR: compiles,
35 passed; 7 failed; 8 ignored(see above — expected given the stale fixture).